feat: add tokio-retry crate to retry failed requests #8
Conversation
There was a problem hiding this comment.
Hi. Thank you for your PR.
At first, please take a look the LogglyClient::send() method (https://github.com/angelcam/rust-slog-loggly/blob/master/src/client.rs#L131). This method will keep trying to send a given message indefinitely if there is an error, so the code that you'd like to add will only retry each individual attempt multiple times. This makes no sense.
If you'd like to change the default behavior from infinite number of attempts to a limited number of attempts with some backoff function, I don't have any problem with that. You'll just need to modify the send method.
Please, make sure that you use cargo fmt and cargo clippy before you commit any code and make sure that all your functions/methods are properly documented.
| pub async fn with_retry<A, F, R, E>(ms: Option<u64>, attempts: Option<usize>, action: A) -> Result<R, E> | ||
| where | ||
| A: FnMut() -> F, | ||
| E: std::fmt::Display, |
There was a problem hiding this comment.
This bound probably isn't necessary. The tokio-retry crate does not require the error type to implement Display and the function itself does not format/display the error.
| time::Duration | ||
| }; | ||
|
|
||
| fn retry_strategy(ms: u64, attempts: usize) -> Take<Map<FibonacciBackoff, fn(Duration) -> Duration>>{ |
There was a problem hiding this comment.
| fn retry_strategy(ms: u64, attempts: usize) -> Take<Map<FibonacciBackoff, fn(Duration) -> Duration>>{ | |
| fn retry_strategy(base: Duration, attempts: usize) -> impl Iterator<Item = Duration> { |
There was a problem hiding this comment.
The function itself probably isn't necessary. It's very simple and it's used only once. It only bloats the code. I'd put the contents directly into with_retry.
| }; | ||
|
|
||
| fn retry_strategy(ms: u64, attempts: usize) -> Take<Map<FibonacciBackoff, fn(Duration) -> Duration>>{ | ||
| FibonacciBackoff::from_millis(ms) |
There was a problem hiding this comment.
| FibonacciBackoff::from_millis(ms) | |
| FibonacciBackoff::from_millis(base.as_millis() as u64) |
|
|
||
| fn retry_strategy(ms: u64, attempts: usize) -> Take<Map<FibonacciBackoff, fn(Duration) -> Duration>>{ | ||
| FibonacciBackoff::from_millis(ms) | ||
| .map(jitter as fn(Duration) -> Duration) |
There was a problem hiding this comment.
| .map(jitter as fn(Duration) -> Duration) | |
| .map(jitter) |
| } | ||
|
|
||
|
|
||
| pub async fn with_retry<A, F, R, E>(ms: Option<u64>, attempts: Option<usize>, action: A) -> Result<R, E> |
There was a problem hiding this comment.
| pub async fn with_retry<A, F, R, E>(ms: Option<u64>, attempts: Option<usize>, action: A) -> Result<R, E> | |
| pub async fn with_retry<A, F, R, E>(base_delay: Duration, attempts: usize, action: A) -> Result<R, E> |
Let's put the defaults into the client module as constants and make them configurable using LogglyClientBuilder.
Added
tokio-retrycrate with awith_retryhelper to retry failed requests. This is useful since request timeouts seem to be happening even with a high timeout value.